coverage: 84.417% (+0.05%) from 84.364%
when pulling 046006a on kbonney:gis_plotting
into c790b61 on USEPA:main.

Pending items to be resolved here or saved for later updates:

• Customize asset markers (valve, pump, reservoir, tank) to match the shapes seen in EPANET. Could borrow code from viswaternet.
• Current approach to showing pump direction as well as pipe direction when directed=True is sensitive to aspect ratio. It looks correct when the aspect ratio is 1, but if you stretch the image in the pop-up viewer the angles of the arrow directions are not updated, so they look off. Another approach could be taken using matplotlib arrow objects to avoid this.
• BUG: An issue arises in the earthquake demo due to an unexpected interaction when using to_gis() on a WaterNetworkModel with leaks. This isn't caused by the code introduced by this PR as far as I can see, rather this interaction wasn't covered by the test suite previously, but now is caused by the use of to_gis in plot_network. See Exception occurs when running to_gis() on a WN with leaks #456 for details.

Hi @meghnathomas, it seems I cannot directly add you as a reviewer to the PR. This probably has to do with some repository level restriction on who can be added as a reviewer. I'll see what I can do to get you added, but let me know if it causes any problems with the review in the meantime.

Failing tests should be addressed when #458 is pulled in.

Will this implementation keep geopandas as an optional dependency, only needed if plotting ? Would be much appreciated if this is the case :-)

Hi, sorry for the delay on the review! Here's some thoughts/comments:

1. For me, often (but not always!) the color bars seem to be normalized to 1 when I use the new geopandas code. E.g., these are the plots I get when I run test_plot_network2 in test_graphics.py (plotting directed and undirected plots of node elevation and link length)

geopandas plotting undirected	networkx plotting undirected

geopandas plotting directed	networkx plotting directed

Similarly, these are the results from test_plot_network4 :

geopandas	networkx

But when we run test_plot_network5, the plots match. The major difference seems to be that we specify "node_range" when we call the function here.

geopandas	networkx

Am I the only one seeing these discrepancies? These are the package version I'm working with on my machine:
matplotlib 3.7.1
geopandas 0.14.4
numpy 1.26.4
pandas 2.2.3
networkx 3.3

2. It might be useful to add a legend (that you can choose to show or hide) for the pumps (triangles? arrows?), valves (a line?), tanks (diamonds?), and reservoirs (squares?) so they can be differentiated easily!

3. The directed graphs are a lot more visually pleasing in the geopandas version. They might be a little easier to visualize if the arrows were slightly smaller, or could somehow be scaled to the fig size. Not a huge deal at all, just noting for future possible enhancements. (looks like you mentioned this in your first comment here on this thread)

4. In the third set of plots above (when we use network Net1), in the geopandas plot the color bars seem to squeeze the network layout a little bit but the networkx version manages to fit the color bars and plots together quite nicely

5. It also looks like for the networkx plots, wntr automatically labels the color bars "Node" or "Link" (which is a nice touch) but that doesn't seem to happen for the geopandas plots

6. It's very cool that you guys plot vertices too now. Looks great!
   

7. Is there any way to disable valve/pump/reservoir/tank plotting? Like for this network, BWFLNet, there are about 500 throttle control valves in the network and the graphic is a little hard to look at:
   

8. Do you also plan to show valve direction (like you show pump direction)?

Hi @meghnathomas, thanks for your rigorous testing of the PR. I'll work through the items you pointed out and get back to you with some updates .

@meghnathomas,

1. Good catch, this should be fixed by the latest commits.
2. I agree this would be helpful. I will have to spend more time looking into this.
3. I agree that the arrows were too large. I changed them so they are more like the size in the networkx version.
4. The cbars should have the correct spacing now.
5. This originally was an intentional choice by the team. I think it looks okay when only plotting one colorbar, but it is confusing if you have both. I'll revisit this with the others.
6. Yes! This PR originated as an effort to add vertices in the networkx framework. It became a pain so it started to make more sense to use the geopandas plotting capabilities, which are much more flexible and user-friendly.
7. I added additional keyword args for this. They default to not show pumps and valves so that previous plotting code doesn't retroactively add them.
8. This should be simple to add. I will look into it.

I also caught a bug where labels only rendered for junctions/pipes rather than all assets.

@kbonney awesome, thanks, everything looks good with the colorbar values, spacing b/w plot and colorbar, arrow sizes, and suppressing pump/valve plotting!

A couple of comments:

1. I think in the networkx version, when alpha < 1, the color bar alpha would match the marker colors alpha, but i think in this geopandas version alpha only applies to the markers and not to the color bar
2. Are the tanks, reservoirs, pumps, and valves supposed to show up by default on all plots, even when you specify node_attribute/link_attribute? Or are they only supposed to show up on a basic network layout plot with no data assigned to nodes/links? If a user specifies "node_attribute" and the list of nodes they are plotting values for doesn't contain the tanks and reservoirs, those nodes don't show up on the map so I don't know where the tanks/reservoirs are located. Is that an intentional choice? If so, that's fine, just wanted to check! Noticed this when I used the following kwargs in your new test_graphics file:

{"node_attribute": node_list},
{"node_attribute": random_node_values},
{"node_attribute": random_node_dict_subset}

I didn't look closely enough at the figures to check if this is true for pumps (and valves) too, but this question applies to them as well

Thanks for the additional comments @meghnathomas:

1. Fixed this, the alpha should now carry over to the cbars.
2. All nodes are always plotted, unless a subset of nodes is specified in which case only that subset is plotted. This includes junctions, tanks, and reservoirs. The only difference is that tanks/reservoirs have a different shape and size.
   This should also be the case for links (pipes, pumps, valves). The edge of a pump or valve is always plotted by default, but the additional visual indicators are only shown when plot_pumps or plot_valves is set to True.

For both nodes and links, all assets should be shown unless a list or Series is passed in as the attribute. In that case, only the asset names that show up in the list or Series index will be shown.

Summary of recent updates:

• Removed pump/valve direction code. This feature could be added back in a later update, but for now I want to prioritize getting a working version of the new function that is backwards compatible.
• When a string is provided as an attribute it is used as the default label.
• Added a legend. The legend chooses some color from the plotted elements as a representative for the symbol. Ideally we would set the color to always be black for consistently, but overriding this behavior has proven difficult.
• All nodes are now plotted regardless of attribute. If a node is not included by the provided attribute the color defaults to black. Similar behavior for links.
• Aspect set to "equal" instead of the default "auto". This forces plots to maintain consistent x/y axis ratio regardless of zoom or the shape of the plotting window.

@kbonney this looks awesome and works super well! :-) Sorry to continue to pester you with comments, but I have just a couple more based on the most recent version of the code:

1. It looks like the colorbar titles can sometimes overlap a little - is that because the aspect ratio is now "auto" you think?
   
2. Thank you for adding the legend! Please feel free to disagree with this comment or leave this task for another time - but it might also be useful to allow users to customize legend location so they can move it around the plot if they want. Providing a default nice legend location that's always out of the way of the network plot no matter what the shape of the network is is also an option but a lot more work on your end probably (that's why we didn't do that for viswaternet haha). In any case this is not critical at all!

Will this implementation keep geopandas as an optional dependency, only needed if plotting ? Would be much appreciated if this is the case :-)

@angusmcb, geopandas would become a required dependency as plotting is considered a core WNTR functionality. Geopandas has had some installation difficulties in the past, but many of those seem to have been taken care of. What is the reason you wouldn't want to install geopandas for WNTR?

I've been working on a plugin for QGIS (currently experimental) which effectively replaces the wntr gis module and geopandas with PyQGIS:
https://github.com/angusmcb/wntr-qgis/

I did think that Geopandas wasn't included as a dependency as standard with QGIS, but have now realised that it is included with the most recent QGIS (windows) releases, so no problem!

I've been working on a plugin for QGIS (currently experimental) which effectively replaces the wntr gis module and geopandas with PyQGIS: https://github.com/angusmcb/wntr-qgis/

I did think that Geopandas wasn't included as a dependency as standard with QGIS, but have now realised that it is included with the most recent QGIS (windows) releases, so no problem!

Thanks for the clarification. It is exciting to hear about the QGIS plugin - looking forward to its progress!